Use in-class initializers, default constructors, class -> struct by chrchr-github · Pull Request #4842 · cppcheck-opensource/cppcheck

chrchr-github · 2023-03-02T14:49:43Z

No description provided.

firewave · 2023-03-02T15:48:38Z

Do we even support this properly in our checks? I remember a mention in a ticket that class initializers are currently not handled at all. We should compare the AST/debug output.

I also have the feeling this should have been done at a point where we don't have like 30 PRs ready to review/merge since this will probably cause several conflicts.

Also we should keep an eye on the performance to make sure some implicit move constructors and move assignment operators do not disappear. #4836 for instance fixed cases where they were missing to begin with.

firewave

Please enable the currently disabled modernize-use-default-member-init and readability-redundant-member-init checks in clang-tidy and also adjust clang-tidy.md. If we are doing this we should make it complete and avoid regressions in future commits.

pfultz2 · 2023-03-02T16:46:37Z

-    bool analyzeTerminate;
+    Analyzer::Action actions{ Analyzer::Action::None };
+    bool analyzeOnly{};
+    bool analyzeTerminate{};


These should be initialized as = false. Using {} will not always zero initialize the memory.

Huh? Do you mean the bool members?

But bool b{}; will always be false.

pfultz2 · 2023-03-02T16:47:17Z

+        explicit Result(Type type) : type(type) {}
        Result(Type type, const Token *token) : type(type), token(token) {}
-        const Token *token;
+        const Token* token{};


This should use = nullptr.

Why? Seems more verbose.

But its more explicit to what it would be initialized to.

If that is our style, then we should enforce it with some kind of check. Otherwise it's just a preference.

For information.. I feel more comfortable with = nullptr also but I have no strong opinion.

firewave · 2023-03-02T18:59:26Z

If something weird/awkward/etc. comes from the clang-tidy results just suppress it and point it out to me so I can have a look and maybe report it upstream. There's a reason some of those have been disabled for so long.

…ppcheck into chr_constructors

firewave · 2023-05-04T09:03:56Z

@@ -1,5 +1,70 @@
 ---
-Checks:          '*,-abseil-*,-altera-*,-android-*,-boost-*,-cert-*,-cppcoreguidelines-*,-darwin-*,-fuchsia-*,-google-*,-hicpp-*,-linuxkernel-*,-llvm-*,-llvmlibc-*,-mpi-*,-objc-*,-openmp-*,-zircon-*,google-explicit-constructor,-readability-braces-around-statements,-readability-magic-numbers,-bugprone-macro-parentheses,-readability-isolate-declaration,-readability-function-size,-modernize-use-trailing-return-type,-readability-implicit-bool-conversion,-readability-uppercase-literal-suffix,-modernize-use-auto,-readability-else-after-return,-modernize-use-default-member-init,-readability-redundant-member-init,-modernize-avoid-c-arrays,-modernize-use-equals-default,-readability-container-size-empty,-bugprone-branch-clone,-bugprone-narrowing-conversions,-modernize-raw-string-literal,-readability-convert-member-functions-to-static,-modernize-loop-convert,-readability-const-return-type,-modernize-return-braced-init-list,-performance-inefficient-string-concatenation,-misc-throw-by-value-catch-by-reference,-readability-avoid-const-params-in-decls,-misc-non-private-member-variables-in-classes,-clang-analyzer-*,-bugprone-signed-char-misuse,-misc-no-recursion,-readability-use-anyofallof,-performance-no-automatic-move,-readability-function-cognitive-complexity,-readability-redundant-access-specifiers,-concurrency-mt-unsafe,-bugprone-easily-swappable-parameters,-readability-suspicious-call-argument,-readability-identifier-length,-readability-container-data-pointer,-bugprone-assignment-in-if-condition,-misc-const-correctness,-portability-std-allocator-const,-modernize-deprecated-ios-base-aliases,-bugprone-unchecked-optional-access,-modernize-replace-auto-ptr,-readability-identifier-naming,-portability-simd-intrinsics,-misc-use-anonymous-namespace,cert-err34-c'
+Checks: >


This format is not supported in older clang-tidy versions which we implicitly still support as we also support older Clang versions.

Who uses our .clang-tidy except us? There is a way to get line-wise formatting in older versions as well, but it's ugly.

Who uses our .clang-tidy except us?

Every user who has an editor which support clang-tidy.

There is a way to get line-wise formatting in older versions as well, but it's ugly.

I know. If possible I would have used that already. The problem is that YAML parser in LLVM is not complete and only supports the bare minimum of features required. So, no.

The multi-line format was not added until about 3 years ago IIRC. We need to do some tests to figure out which version that was. As ubuntu 20.04 is definitely one of the operating systems we should properly support as a dev system it is quite possible we slightly missed that.

Another problem is, that it fails silently. So it will just ignore the option and will use the default checks.

They also just recently changed the default format to more modern YAML which is also not backwards compatible.

Checks: ' ,*, ,-bugprone-assignment-in-if-condition, ,-bugprone-easily-swappable-parameters, '

This is what I meant above, it should work with older clangs.
But even if there were developers (not users) on older systems, using an old clang makes little sense, since we have the current version in our CI, which needs to pass eventually.

This is what I meant above, it should work with older clangs.

I will give it a spin later today. I have several older versions installed.

But even if there were developers (not users) on older systems, using an old clang makes little sense, since we have the current version in our CI, which needs to pass eventually.

I think using any LTS version is justified. ubuntu 20.04 (still supported until at least 2025) comes with Clang 10 out of the box and it seems there's also an official repo with Clang 13. But distros rarely update such major packages after release. There's also the possibility to install the latest via the LLVM repo. But it also depends on your IDE integration. But the length of the LTS support windows have been getting quite ridiculous. I happy that non-LTS versions usually get discarded as soon as the next non-LTS ones are out. But unfortunately that solidifies the usage of older LTS versions.

Just take a look at the LLVM bug tracker. They frequently get reports for older versions.

Or ubuntu Touch - they have been working for years now to move off ubuntu 16.04 and just recently achieved their move to ubuntu 20.04. It is unlikely that their CI is using the latest compilers.

We really need to open up the discussions here so we don't go off-topic so often.

We really need to open up the discussions here so we don't go off-topic so often.

any reason to not use the forum on sourceforge? We don't need several forums.

Since all development happens on Github, we might also close Sourceforge and focus on a single site. Probably more people are already on Github than on Sourceforge.

Every user who has an editor which support clang-tidy.

That sounds a lot. But cppcheck users will not see this file. This is not installed.

I would say it's only cppcheck developers that might use this. And well most developers will likely not use this file neither locally. I assume it makes sense to use it locally only if you have the same clang-tidy version as CI because there are different checkers in different versions.

firewave · 2023-05-04T09:08:13Z

+        const Token* vartok{};
        std::list<ValueFlow::Value> true_values;
        std::list<ValueFlow::Value> false_values;
        bool inverted = false;


Why explicit false here?

AFAIK we have not yet decided on a mandatory convention for initializers.

firewave · 2023-05-04T09:10:21Z

Something I mentioned before. We should check that this kind of code is actually (properly) supported by us. Would be bad if this would cause us to miss issues in the selfcheck from now on. That should prevent this from being merged though.

I would also like to see an approval from @pfultz2 .

chrchr-github · 2023-05-04T09:50:58Z

Something I mentioned before. We should check that this kind of code is actually (properly) supported by us. Would be bad if this would cause us to miss issues in the selfcheck from now on. That should prevent this from being merged though.

The only issue that I'm aware of is that side effects in initializers are not considered (https://trac.cppcheck.net/ticket/11205). But we don't have in this PR.

firewave · 2023-05-04T10:04:34Z

The only issue that I'm aware of is that side effects in initializers are not considered (https://trac.cppcheck.net/ticket/11205). But we don't have in this PR.

Great!

I was afraid that initializations with POD types like bool b{} or void *p{} might not result in the proper values being assigned. But seems that is not supported even outside of classes:

void f() {
    bool b1{};
    if (b1) {} // no warning

    bool b2 = false;
    if (b2) {}

    void* p1{};
    if (p1) {} // no warning

    void* p2 = nullptr;
    if (p2) {}
}

chrchr-github · 2023-05-04T10:25:53Z

See https://trac.cppcheck.net/ticket/10358

chrchr-github · 2023-08-04T12:51:31Z

So, how about this?

danmar · 2023-08-07T17:45:13Z

So, how about this?

There is a conflict now. But if you solve that feel free to merge. I find it hard to determine that there is no changed behavior anywhere though. But overall I am fine with this refactoring 👍

Concerns about .clang-tidy have been addressed.

firewave · 2023-08-08T14:49:57Z

We should check how this affects our AST to maybe find some shortcomings.

I am also a bit creeped out that you enums with invalid values this way. I haven't looked into that yet.

chrchr-github added 23 commits March 1, 2023 19:05

Use in-class initializers, default constructors, class -> struct

dacd2fa

Format

8be07e0

Missing init, more constructors

cd89650

Fix compiler error

cdb56ba

Missing init

85f2632

Missing init

f3d693e

Restore init

8a9424b

Duplicate

18b2ff5

More constructors, rule of zero

245889a

More constructors

d506867

Fix init

206de7d

Undo

b5278c9

More constructors

500ca9a

Format

1784841

Workaround for internalAstError

549a016

Fix compiler error

d6e38fe

clang-tidy

b5ed91e

More workarounds

081402e

Another enum fix

f0a292b

Amend

b8f2095

Amend again

06704f2

Enum fix

8895983

Enum fix

22a6bf6

firewave self-requested a review March 2, 2023 15:54

firewave requested changes Mar 2, 2023

View reviewed changes

chrchr-github marked this pull request as draft March 2, 2023 16:05

Enable clang-tidy checks

b735c8f

pfultz2 reviewed Mar 2, 2023

View reviewed changes

chrchr-github added 6 commits April 10, 2023 21:13

Merge branch 'chr_constructors' of https://github.com/chrchr-github/c…

ce5aedc

…ppcheck into chr_constructors

Merge, format .clang.tidy

8890000

Merge

b66c9a1

Use = 0

59fec1f

Format

6e8e0ee

Fix .clang-tidy

fea2f46

danmar approved these changes May 4, 2023

View reviewed changes

firewave previously requested changes May 4, 2023

View reviewed changes

chrchr-github and others added 10 commits May 4, 2023 20:05

Merge

d0d1be4

Merge

043226b

Merge branch 'danmar:main' into chr_constructors

4fc5cba

Merge

48bba24

Merge

5d913dd

Fix merge

97193c4

Fix merge

1993756

Merge

45e1d15

Merge

60666f1

Merge

3f73305

chrchr-github added 2 commits August 7, 2023 22:50

Merge

44fe7f2

Merge

a6f613d

chrchr-github merged commit eee1221 into cppcheck-opensource:main Aug 8, 2023

chrchr-github deleted the chr_constructors branch August 8, 2023 09:05

Conversation

chrchr-github commented Mar 2, 2023

Uh oh!

firewave commented Mar 2, 2023

Uh oh!

firewave left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

firewave commented Mar 2, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

firewave commented May 4, 2023

Uh oh!

chrchr-github commented May 4, 2023

Uh oh!

firewave commented May 4, 2023

Uh oh!

chrchr-github commented May 4, 2023

Uh oh!

chrchr-github commented Aug 4, 2023

Uh oh!

danmar commented Aug 7, 2023

Uh oh!

firewave commented Aug 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

firewave left a comment •

edited

Loading